Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IN_CLOSE_WRITE Event #93

Merged
merged 1 commit into from
Aug 14, 2016
Merged

Add IN_CLOSE_WRITE Event #93

merged 1 commit into from
Aug 14, 2016

Conversation

get200
Copy link
Contributor

@get200 get200 commented Aug 13, 2016

The IN_MODIFY event is emitted on a file content change (e.g. via the write() syscall) while IN_CLOSE_WRITE occurs on closing the changed file. It means each change operation causes one IN_MODIFY event (it may occur many times during manipulations with an open file) whereas IN_CLOSE_WRITE is emitted only once (on closing the file).

@dfaust
Copy link
Member

dfaust commented Aug 14, 2016

Thank you for your patch.
I think it makes sense to forward IN_CLOSE_WRITE since it is a very useful event.
The biggest problem with IN_CLOSE_WRITE is that it is only available with inotify. But given that it can be very useful, it might be worth adding anyway. Though it has to be made clear in the docs that it is only available on Linux.
Also the tests need to be updated (I can do that if you want).
And some more nitpicking: I would call it CLOSE_WRITE (with an underscore) and you should add a changelog entry.
What do you think, @passcod ?

@get200
Copy link
Contributor Author

get200 commented Aug 14, 2016

ok!

@get200 get200 closed this Aug 14, 2016
@dfaust
Copy link
Member

dfaust commented Aug 14, 2016

@xieke91 Do you know that you can update your pull requests, you don't have to close them and open new ones.
Just push new commits to your forked repository and they will show up in the pull request.

And if you want to have a nice looking git log, you can use git rebase to rewrite your local history and use git push --force to forcefully push your changes to your forked repository.

@dfaust dfaust reopened this Aug 14, 2016
@get200
Copy link
Contributor Author

get200 commented Aug 14, 2016

sorry,I am not very familiar with git and I'm already update it with CLOSE_WRITE.

@get200
Copy link
Contributor Author

get200 commented Aug 14, 2016

I try to edit tests but something wrong in windows and macos,maybe you can do it.

@dfaust
Copy link
Member

dfaust commented Aug 14, 2016

I don't think that the test that's failing in travis build nr. 215.1 has anything to do with your patch. I'll have a look at that.
I think it looks ok, but since I'm not the maintainer, I won't merge it, lets see what @passcod has to say.

@passcod passcod added this to the 3.0.0 milestone Aug 14, 2016
@passcod
Copy link
Member

passcod commented Aug 14, 2016

Yep, looks good. I think kqueue (BSD's inotify) may support an analog to IN_CLOSE_WRITE, so it may not be a linux-only thing in the long term.

@Hessijames I'll have a closer look (finally! it's been a busy few weeks) at debounce, but do you think CLOSE_WRITE could be integrated somehow? Like, can we have some way of using CLOSE_WRITE on linux and a debounced MODIFY on other platforms to watch for "a file has been modified"? Just thinking out loud, it's probably infeasible, hah.

@passcod passcod merged commit ff24a36 into notify-rs:master Aug 14, 2016
@dfaust
Copy link
Member

dfaust commented Aug 15, 2016

I thought about that as well, but in the end I think it would only complicate things. The debounce module debounces more than just write events (eg. "safe saves"), so using CLOSE_WRITE would introduce inconsistencies not only across platforms but also across events.
Actually I think the use-cases for CLOSE_WRITE in notify are quite narrow. But including it doesn't hurt much and it gives developers the possibility to implement such a "custom debounce" logic if they really need it.

@passcod
Copy link
Member

passcod commented Aug 16, 2016

That makes sense 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants